Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[order,matcher]: Define the Order type and begin matcher. #15

Merged
merged 11 commits into from
Sep 7, 2019

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Aug 23, 2019

Warning! This adds several PLACEHOLDER/TODO sections.

Deviations from spec (updated via #16):

  • hash func is blake256, not sha256. Easily changed.
  • market orders do not have time-in-force, only limit orders do.

Code

market/order:

  • Create the Order interface under the server/market/order package.
  • Create the OrderID type.
  • Implement order serialization and order ID calculation for cancel, market, and limit orders.
  • Define a UTXO interface for providing the UTXO data field of the orders. This may change.
  • 88.1% test coverage of the order package.

matcher:

  • Add sortQueue and shuffleQueue functions, to be used in order matching.
  • Define a barebones Matcher type.
  • Implement the (*Matcher).Match method and the unexported helper functions.:
func (m *Matcher) Match(book Booker, queue []order.Order) (matches []*order.Match, passed, failed []order.Order)
  • Define the Booker interface for interacting with the order book.
  • 90.7% test coverage of the matcher package

account: Primarily a placeholder, but create an AccountID type with a constructor to double-hash a pubkey as per spec.

account/pki: Primarily a placeholder. Just define PrivateKey in terms of secp256k1, and pub/priv key size consts similarly.

market: Entirely a placeholder. Create a stub Market type with comments outlining the primary intended functions. Hint at the need for a concrete EpochQueue type, which should either implement some interface defined by the matcher package, or have a primitive field of type []order.Order that can be provided as a concrete type to the matcher.

@chappjc chappjc requested a review from buck54321 August 23, 2019 15:37
server/asset/asset.go Outdated Show resolved Hide resolved
server/asset/asset.go Outdated Show resolved Hide resolved
server/market/order/order.go Outdated Show resolved Hide resolved
server/matcher/match.go Show resolved Hide resolved
server/matcher/match_test.go Outdated Show resolved Hide resolved
server/market/order/order_test.go Show resolved Hide resolved
@chappjc chappjc marked this pull request as ready for review August 26, 2019 18:21
@chappjc chappjc changed the title [order,matcher]: Define the Order type and begin matcher. WIP [order,matcher]: Define the Order type and begin matcher. Aug 27, 2019
Warning! This adds several PLACEHOLDER/TODO sections.

Deviations from spec:
 - hash func is blake256, not sha256. Easily changed.
 - market orders do not have time-in-force, only limit orders do.

market/order: Create the Order type under the server/market/order
package. Implement order serialization and order ID calculation.

matcher: Add sortQueue and shuffleQueue functions, to be used in order
matching. The Matcher type and its Match method are placeholders.

asset: Primarily a placeholder, but create an AssetType enum and UTXO
type for use in the order.Order type.

account: Primarily a placeholder, but create an AccountID type with
a constructor to double-hash a pubkey as per spect.

account/pki: Primarily a placeholder.  Just define PrivateKey in terms
of secp256k1, and pub/priv key size consts similarly.

book: Entirely a placeholder. When the matcher.Booker interface is
completely specified, the OrderBook methods can be implemented.

market: Entirely a placeholder. Create a stub Market type with comments
outlining the primary intended functions. Hint at the need for a
concrete EpochQueue type, which should either implement some interface
defined by the matcher package, or have a primitive field of perhaps
[]*order.Order that can be provided as a concrete type to the matcher.
Add Filled field to MarketOrder (and LimitOrder).

Remove OrderSide enum and use a Sell bool instead.
@chappjc chappjc changed the title WIP [order,matcher]: Define the Order type and begin matcher. [order,matcher]: Define the Order type and begin matcher. Aug 31, 2019
@chappjc
Copy link
Member Author

chappjc commented Aug 31, 2019

Completed test coverage of order and matcher packages.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of questions, otherwise good to go.

server/market/order/order.go Outdated Show resolved Hide resolved
type Prefix struct {
AccountID account.AccountID
BaseAsset uint32 // must be 0 for CancelOrder
QuoteAsset uint32 // must be 0 for CancelOrder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on how these uint32 asset IDs might be generated? In the spec, they are ASCII-encoded ticker symbols. The reasoning there was that the ticker symbol is unique and lends itself to auditing. The downside is that they are variable length.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine the DEX will be in charge of mapping the integer identifier to the ticker symbol string. Perhaps the "asset object" returned to the client in response to the assets RPC in https://github.com/decred/dcrdex/blob/master/spec/README.mediawiki#exchange-variables can be updated with the integer ID.

My thinking about this Prefix struct is that it's primary purpose is byte serialization for computing order ID and storing the order data compactly, while the ticker symbol strings would come in with JSON serialization, and that would be something handled by something higher up like the dex or communications hub.

I suppose I'd be open to having these fields as arrays (i.e. [5]byte), but I wanted to avoid a string here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[5]byte is interesting. We need it to be repeatable for future mesh interoperability. If we wanted an int, we could potentially use the BIP-0044 coin type index.

https://github.com/satoshilabs/slips/blob/master/slip-0044.md

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BIP-0044 coin types makes a lot of sense. I'd go with that as a convention unless there are strong architectural benefits of putting the ticker characters in the order.Prefix object, but I think the ticker mapping can exist at a higher level in the dex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #29

Copy link
Member Author

@chappjc chappjc Sep 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An itch that I still can't quite scratch is that these two asset IDs don't apply to a cancel order, or don't have to at least.

  • Should cancel order prefixes be validated to ensure these values are as expected for the order in question (not much point of that)?
  • Should they be required to both be some other sentry value like uint32(-1)?
  • Should the asset types be moved from the prefix to the MarketOrder (and thus LimitOrder too)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see it either way. I like to think that if I was handed a serialized cancel order, I would be able to tell what market it's for, but you're right that it's not a necessity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we'll set these asset types for cancel orders, but I'm not sure if we'll want to bother validating them for incoming cancel orders.

server/market/order/order.go Outdated Show resolved Hide resolved
server/matcher/match.go Outdated Show resolved Hide resolved
server/matcher/match.go Outdated Show resolved Hide resolved
server/matcher/match_test.go Show resolved Hide resolved
server/market/order/order.go Outdated Show resolved Hide resolved
Checking the quote asset quantity of market buy orders against lot size,
best sell order rate, and the fractional market buy buffer should occur
only when validating incoming orders, not in the matching process.
Price rate is atoms of quote asset, applied per 1e8 units of the base asset.

Add matcher.BaseToQuote and matcher.QuoteToBase to perform the big
integer math with an integer rate.
@chappjc chappjc merged commit c48f5b8 into decred:master Sep 7, 2019
@chappjc chappjc deleted the matcher branch September 7, 2019 17:10
@chappjc chappjc mentioned this pull request Sep 9, 2019
@chappjc chappjc added the server server components label Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server server components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants